-
Notifications
You must be signed in to change notification settings - Fork 424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add support for ManagedMediaSource 'startstreaming' and 'endstream' event handling #1542
base: main
Are you sure you want to change the base?
Conversation
…ing event handling
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1542 +/- ##
==========================================
+ Coverage 86.36% 86.37% +0.01%
==========================================
Files 43 43
Lines 11148 11165 +17
Branches 2545 2548 +3
==========================================
+ Hits 9628 9644 +16
- Misses 1520 1521 +1 ☔ View full report in Codecov by Sentry. |
@@ -192,6 +192,7 @@ export class PlaylistController extends videojs.EventTarget { | |||
this.playlistExclusionDuration = playlistExclusionDuration; | |||
this.maxPlaylistRetries = maxPlaylistRetries; | |||
this.enableLowInitialPlaylist = enableLowInitialPlaylist; | |||
this.hasManagedMediaSource_ = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: To save a few lines of code, you could change this to:
this.hasManagedMediaSource_ = experimentalUseMMS && window.ManagedMediaSource;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent of the flag was to signal that the ManagedMediaSource has been created and currently exists. If we set it here we should rename the flag to useManagedMediaSource_
or createManagedMediaSource_
, as it hasn't been created yet.
@@ -215,6 +216,8 @@ export class PlaylistController extends videojs.EventTarget { | |||
// Airplay source not yet implemented. Remote playback must be disabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can change the above if statement to:
if (this.hasManagedMediaSource_) {
@@ -232,6 +235,14 @@ export class PlaylistController extends videojs.EventTarget { | |||
// we don't have to handle sourceclose since dispose will handle termination of | |||
// everything, and the MediaSource should not be detached without a proper disposal | |||
|
|||
if (this.hasManagedMediaSource_) { | |||
this.handleStartStreaming_ = this.handleStartStreaming_.bind(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add the content of this if statement to the above pre-existing if statement. All nits, but saves a few lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean to the if (experimentalUseMMS && window.ManagedMediaSource) {...}
block? It is more concise but I opted against it because to my eye it made the code slightly less organized by putting the new addEventListener
calls inside the block where the MediaSource is set rather than in the subsequent step/section where the other MediaSource event handlers are added. Minor issue, I'm happy to change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, all my comments were basically to save some lines of code by moving this code to where you set the hasManagedMediaSource
flag and making the flag unnecessary, but fine with me if it keeps it cleaner in your eyes. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! Looks good to me
@@ -232,6 +235,14 @@ export class PlaylistController extends videojs.EventTarget { | |||
// we don't have to handle sourceclose since dispose will handle termination of | |||
// everything, and the MediaSource should not be detached without a proper disposal | |||
|
|||
if (this.hasManagedMediaSource_) { | |||
this.handleStartStreaming_ = this.handleStartStreaming_.bind(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, all my comments were basically to save some lines of code by moving this code to where you set the hasManagedMediaSource
flag and making the flag unnecessary, but fine with me if it keeps it cleaner in your eyes. 👍
Description
This PR adds onto the experimental support for
ManagedMediaSource
. When the user agent determines that a ManagedMediaSource has enough data buffered to ensure uninterrupted playback, or does not have enough to ensure it, it will fire aendstreaming
orstartstreaming
event, indicating that the player should stop loading new segments or start loading new segments, respectively.Specific Changes proposed
Add 2 new event handlers for
startstreaming
andendstreaming
. The former will callplaylistController.load()
, which was a preexisting method that callsload()
on all active segment loaders. The latter callsplaylistController.pause()
, which is a new method that callspause()
on all active segment loaders.Requirements Checklist
References
https://www.w3.org/TR/media-source-2/#dom-managedmediasource-streaming
w3c/media-source#320